-
Notifications
You must be signed in to change notification settings - Fork 605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set AWS container properties from container options #2471
Conversation
…tainer properties. Signed-off-by: Manuele Simi <manuele.simi@gmail.com>
Signed-off-by: Manuele Simi <manuele.simi@gmail.com>
Signed-off-by: Manuele Simi <manuele.simi@gmail.com>
plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AWSContainerOptionsMapper.groovy
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Manuale, this is welcome. Actually, I had gave a try some time ago but gave up when I realised container options should be managed at Job Definition level (what a design mistake ..)
My main comment is that this is a good opportunity to manage the containerOption
as a map. Look at my commit that I've just pushed
Great work (and fast!). |
I would say to merge my branch in your PR, then you can continue with that |
…container_properties
@pditommaso There is a problem with your parser. The Map doesn't consider options that can be repeated multiple times (like --env or --tmpfs). Each option should be mapped to a List of values. |
Also, '=' cannot be used as separator in the |
It's getting complicated. Should repeatable options be supported? my original plan was only to support a subset of them. |
Parsing command line options is never easy. Unfortunately I made a change that seems to work: Would that be OK with you? What's probably more concerning is that we should support only the format "option value" (which is the one documented in the docker run help), not "option=value". So a value with an equal char is not separated (like in the screenshot above). |
Is basically a |
yes, it is. |
Maybe we should model it as a |
I'd go for a custom holder where we can add the lookup methods we will need. |
👍 |
Question: The method
Which one would you favor? |
I would say 1. |
Also a combination of 1. and 2. would work well. |
Sounds good. |
As you wish 😄 |
…ptions. Signed-off-by: Manuele Simi <manuele.simi@gmail.com>
plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsContainerOptionsMapper.groovy
Show resolved
Hide resolved
plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsContainerOptionsMapper.groovy
Show resolved
Hide resolved
plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsBatchTaskHandlerTest.groovy
Show resolved
Hide resolved
plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsBatchTaskHandler.groovy
Show resolved
Hide resolved
Just realised that
nextflow/plugins/nf-amazon/src/test/nextflow/cloud/aws/batch/AwsContainerOptionsMapperTest.groovy Lines 12 to 156 in 80dbfa8
|
It is also tested there. When you asked for a test, I thought you wanted a dedicated test class. I agree and will adjust the tests as you suggested. |
Signed-off-by: Manuele Simi <manuele.simi@gmail.com>
Signed-off-by: Manuele Simi <manuele.simi@gmail.com>
done, see d1d9da0 |
I think I've addressed your latest comments. Pls, let me know if I missed something. Thanks! |
Hold on, there is a test failing. I know why. I can fix it later. |
Signed-off-by: Manuele Simi <manuele.simi@gmail.com>
I changed two |
...-google/src/test/nextflow/cloud/google/lifesciences/GoogleLifeSciencesTaskHandlerTest.groovy
Outdated
Show resolved
Hide resolved
modules/nf-commons/src/main/nextflow/util/CmdLineOptionMap.groovy
Outdated
Show resolved
Hide resolved
plugins/nf-amazon/src/main/nextflow/cloud/aws/batch/AwsContainerOptionsMapper.groovy
Outdated
Show resolved
Hide resolved
Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terrific! This was much needed for our AWS projects. |
Released along with version 21.12.1-edge |
This commit adds the support for `containerOptions` directive for AWS Batch jobs This is useful to fine control the container execution properties using the same the option used for the Docker container runtime. Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com> Co-authored-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
This contribution addresses the requirements from #2282 and #2413.
With this change, the container properties of a job definition registered by Nextflow in AWS Batch are set starting from the container options defined in the Nextflow task.
Example
Given the following container options:
the job is registered with the following properties on AWS Batch:
Do note that: